Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[a11y] Announce new tab/pane upon creation #13575

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[a11y] Announce new tab/pane upon creation #13575

wants to merge 5 commits into from

Conversation

carlos-zamora
Copy link
Member

Summary of the Pull Request

Makes the automation peer for the TerminalPage announce that a new tab/pane was created successfully. In order to do this, we need to store the automation peer for TerminalPage. Then we have easy access to dispatch notifications from it.

Closes #12038

Validation Steps Performed

Open a new tab/pane with Narrator active.

@ghost ghost added Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Jul 22, 2022
@github-actions

This comment has been minimized.

Sign in to view
@carlos-zamora
Copy link
Member Author

  • I could add a similar notification for when the settings tab is opened. I wasn't sure if that was desired though?
  • I originally tried having this be a part of TerminalTab and Pane, but for some reason the TabViewItem's automation peer would be ignored when it dispatched a notification event.

@carlos-zamora
Copy link
Member Author

oops, doesn't work for duplicate tab. Should be easy to fix. I should check duplicate pane too.

@github-actions

This comment has been minimized.

Sign in to view
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/a11y-sev3/new-profile-announcement branch from 38ac9c5 to 0473430 Compare July 25, 2022 17:31
@carlos-zamora carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Jul 25, 2022
@ghost ghost requested review from zadjii-msft, PankajBhojwani and DHowett July 25, 2022 19:17
@carlos-zamora
Copy link
Member Author

@msftbot merge this in 10 minutes

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 26, 2022
@ghost

This comment was marked as outdated.

Sign in to view
DHowett
DHowett requested changes Jul 26, 2022
View reviewed changes
src/cascadia/TerminalApp/TabManagement.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/Resources/en-US/Resources.resw
Comment on lines +750 to +754
<value>A new tab for the "{}" profile was created successfully</value>
<comment>{} will be replaced with the name of the profile (i.e. PowerShell, Command Prompt, etc.). The name may span multiple words. This announcement is read by a screen reader.</comment>
</data>
<data name="SplitPaneAnnouncement" xml:space="preserve">
<value>A new pane for the "{}" profile was created successfully</value>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the wording here throws me a bit (feels clunky).. gut check how it sounds?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A11y team requested: "profile name created successfully"

We've diverged a bit since then haha. I find value in explicitly saying "pane" or "tab". I also think saying "profile" is important because some profile names could be weird. That said, what if we change it to...
"{}" pane/tab created successfully

It's more succinct? But there's still some doubts when it comes to localization. Thoughts?

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 26, 2022
@DHowett DHowett removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 26, 2022
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 27, 2022
@carlos-zamora carlos-zamora removed the Needs-Second It's a PR that needs another sign-off label Jul 27, 2022
zadjii-msft
zadjii-msft reviewed Jul 29, 2022
View reviewed changes
src/cascadia/TerminalApp/TerminalPage.cpp
@@ -630,6 +635,11 @@ namespace winrt::TerminalApp::implementation

if (auto page{ weakThis.get() })
{
page->_processingCommandlineArgs = true;
auto finishProcessing = wil::scope_exit([&page]() {
page->_processingCommandlineArgs = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.... Can't you just use _startupState <= StartupState::Initialized instead of introducing a new member?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No because we're trying to suppress notifications when processing commandline args regardless of initialization step. So your proposal works for wt.exe args from Run, but not from commandline mode in the command palette.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, i am of two minds. I don't know if I would be surprised if they were suppressed if I just typed in the command that made it happen.

@zadjii-msft zadjii-msft added this to the Terminal v1.17 milestone Aug 1, 2022
@codeofdusk
Copy link
Contributor

NVDA will always filter out notifications from terminal (see nvaccess/nvda#13261). Are these being sent by the TermCtrl provider?

@carlos-zamora
Copy link
Member Author

NVDA will always filter out notifications from terminal (see nvaccess/nvda#13261). Are these being sent by the TermCtrl provider?

These aren't sent by TermCtrl provider. They're sent by a provider for the whole window, which manages the tabs and UI outside/above the terminal area itself.

Tested this with NVDA and NVDA does read out these notifications, probably for the reason above.

@carlos-zamora carlos-zamora requested a review from DHowett August 16, 2022 21:09
DHowett
DHowett requested changes Aug 25, 2022
View reviewed changes
src/cascadia/TerminalApp/AppActionHandlers.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/AppActionHandlers.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp
@@ -630,6 +635,11 @@ namespace winrt::TerminalApp::implementation

if (auto page{ weakThis.get() })
{
page->_processingCommandlineArgs = true;
auto finishProcessing = wil::scope_exit([&page]() {
page->_processingCommandlineArgs = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, i am of two minds. I don't know if I would be surprised if they were suppressed if I just typed in the command that made it happen.

src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Aug 25, 2022
@carlos-zamora carlos-zamora requested a review from DHowett August 25, 2022 23:48
@zadjii-msft zadjii-msft removed this from the Terminal v1.17 milestone Jul 5, 2023
@carlos-zamora carlos-zamora mentioned this pull request Jul 27, 2023
Merged
microsoft-github-policy-service bot pushed a commit that referenced this pull request Aug 25, 2023
@carlos-zamora
Make screen reader announce successful MovePane and MoveTab actions (#…
2fb4a7f 
…15771)

Uses the `RaiseNotificationEvent()` API from UIA automation peers to
announce successful `MovePane` and `MoveTab` actions. The announcements
are localized in the resw file.

Closes #15159
Based on #13575
DHowett pushed a commit that referenced this pull request Sep 22, 2023
@carlos-zamora @DHowett
Make screen reader announce successful MovePane and MoveTab actions (#…
eff8cfa 
…15771)

Uses the `RaiseNotificationEvent()` API from UIA automation peers to
announce successful `MovePane` and `MoveTab` actions. The announcements
are localized in the resw file.

Closes #15159
Based on #13575

(cherry picked from commit 2fb4a7f)
Service-Card-Id: 90438494
Service-Version: 1.18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@lhecker lhecker lhecker approved these changes

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

@DHowett DHowett Awaiting requested review from DHowett

Requested changes must be addressed to merge this pull request.

Assignees

@DHowett DHowett

Labels
Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
5 participants
@carlos-zamora @codeofdusk @DHowett @lhecker @zadjii-msft